This seems like it should take a Signal, not a Property. 😕

It should be easy to go from the property to the signal, but it's not easy to go the other way.

Hmm, I think I kinda see it the other way. If this was implemented in terms of Signal, we'd have to pick a default starting state for the throttle, probably .resumed so that events are passed through. Using Property means we don't have to make that assumption.

From a slightly different perspective, Property is more powerful than Signal in that you could implement a signal-based overload of this method by composing it with an initial value and calling through to the property-based implementation.

I pushed 4dcd2bf to demonstrate what I mean. If you think those overloads would be useful I'll add some docs and merge those changes in here.

You probably meant skipRepeats here, not uniqueValues?

Also, this should be formatted according to the RAC style.

Fixed.

I think this value should get sent outside of the Atomic's lock to prevent deadlock.

Even though it's in a schedule block, that can execute synchronously if, e.g., you're on the main queue and you're using the UIScheduler.

Same goes for the send below.

Made this change in e2dd090, let me know what you think.

This could just be return self == .terminated.

That requires explicit Equatable conformance of ThrottleWhileState since case throttled has an associated value.

Stylistically, this could also become a switch statement if you prefer, which would add exhaustively checking.

which would add exhaustively checking.

And fail to compile if one adds a new case, which is a good thing

The returned value from an action of Atomic.modify is not a new value for the Atomic instance since ReactiveCocoa/ReactiveCocoa#2984. So this should be written as follows:

let valueToSend = state.modify { state -> Value? in
    guard !state.isTerminated else { return nil }

    if shouldThrottle {
        state = .throttled(nil)
        return nil
    } else {
        state = .resumed
        if case let .throttled(value) = state {
            return value
        } else {
            return nil
        }
    }
}

Good call! I've pushed 58e5bed. I ended up having a single return nil after the conditional block, but if everyone prefers this style (a return at every branch) I can change it.

I ended up having a single return nil after the conditional block

👍

Same as https://github.com/ReactiveCocoa/ReactiveSwift/pull/58/files#r82397528:

let eventToSend = state.modify { state -> Event<Value, Error>? in
    switch event {
    case let .value(value):
        switch state {
        case .throttled:
            state = .throttled(value)
            return nil
        case .resumed:
            return event
        case .terminated:
            return nil
        }

    case .completed, .interrupted, .failed:
        state = .terminated
        return event
    }
}

You don't need to unwrap the bound value with ? since the return type is Value?: case .throttled(let value).

We should consistently place let right after the case: if case let .throttled(value).

I've moved the let outside 👍

You don't need to unwrap the bound value with ? since the return type is Value?

While that's technically true, I think there's value in being precise with exactly the conditions where this branch should be followed. The idea is that we should only send value event if a value was actually throttled, and I feel like this version communicates that more clearly. What do you think?

Same as https://github.com/ReactiveCocoa/ReactiveSwift/pull/58/files#r82402862.

Could return the original event here?

👍

This initial check seems unnecessary, since shouldThrottle.producer below would emit the current value.

@sharplet Hmm, may I ask what's the specific reason to retain it? I don't think the resulting signal should own shouldThrottle in any way.

What if it terminates the resulting signal when shouldThrottle completes, gaining the take(during:) semantic?

In my mind it's the same reason that Action captures a strong reference to its enabledIf property. So to me it seems convenient, not having to maintain an extra strong reference to that property.

But that doesn't help me to understand what the problem with that strong reference is.

What if it terminates the resulting signal when shouldThrottle completes, gaining the take(during:) semantic?

Regardless of the current behaviour, are you saying that if shouldThrottle completes, the returned signal should or should not also complete? I think it would make sense to add some tests for these semantics so it's clear what we want the behaviour to be.

An observation to the push-based Signal should not have any effect on the event stream. If we treat operator as specialised observations, and Propertys as specialised Signals with stronger guarantees, by the logic a signal operator that takes a Property should not cause any side effect on its sources.

Now retaining the property violates this, as the retainment influences the termination of the stream (the property) that is supposed to just be observed.

IIRC this would be the first ever signal operator that has a side effect upon the upstream signal if merged.

Action is a different story, since it is not a stream of value itself, but a state machine that owns a few streams.

Regardless of the current behaviour, are you saying that if shouldThrottle completes, the returned signal should or should not also complete?

Either would do, but completing with shouldThrottle's completion sounds a fit. Say for a Swift equivalent of ReactiveViewModel, I think it makes sense to have these throttled signals gone together with the supplied shouldThrottle property (which is owned by the view model).

It seems to me like the options are:

1. When shouldThrottle completes, the signal may get stuck in the throttled state (i.e., whichever state was last)
2. When shouldThrottle completes, the state gets switched back to .resumed
3. When shouldThrottle completes, the signal completes

(2) feels plain wrong to me, which makes it a toss-up between (1) and (3). So I feel like it comes down to which option is more surprising, and which option is easier to see/debug. For that reason I'm leaning towards tying the lifetime of the returned signal to the lifetime of shouldThrottle.

@mdiep What do you think?

Or anyone else who's been involved in the thread of course! 😅

(1) seems like the right thing to me, although it's a tough call.

That seems like the least surprising thing, and most in line with other operators. (c.f. sample, which only completes when both signals have completed)

We should definitely add tests for this—and probably for an interrupt on the signal as well.

This keeps a strong reference to shouldThrottle for SignalProducers. @andersio does this need to change as well?

The first thing I tried was to wrap shouldThrottle in an existential:

let shouldThrottle = Property(shouldThrottle)
return lift { $0.throttle(while: shouldThrottle, on: scheduler) }

But that also increases the lifetime of the underlying property. Is there any way to capture a reference to this property that won't extend its lifetime? It looks like composing properties fundamentally alters the lifetime of the source property, extending it to be the union of all transitive property lifetimes. Is that intentional?

That is intentional IIRC. All composed properties derived from a property is considered just a reduced/transformed view of that property, so that the behaviour is in line with existentials which retain the wrapped property (like the one in your example).

That's said it might be worth discussing if the source capturing should be dropped. It could be an oversight (especially with the arguments I made in #58 (comment)... ehm), since in ReactiveCocoa/ReactiveCocoa#2922 @mdiep and I focused just on how signal and producer should behave.

Edit: Edited many times.

Edit 2: It seems the capturing is originated from ReactiveCocoa/ReactiveCocoa#2788. The argument to capture was different though, and it was actually invalidated by ReactiveCocoa/ReactiveCocoa#2922...

Edit 3: Opened a PR. #117

#117 is merged. Now this is the last stop before RC1 (hopefully). 🎉

Is 4 notes excessive? 😅

Perhaps just two notes (shouldThrottle + termination behavior), or no notes at all?